[OSPP]Support Kubernetes ConfigMap for Apollo java, golang client #79
Conversation
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
WalkthroughThe changes introduce Kubernetes integration into the Apollo client, including a new dependency on the Kubernetes Java client library. New classes and methods are added to manage Kubernetes ConfigMaps, allowing for the creation, reading, updating, and existence checking of ConfigMaps. The configuration management system is enhanced with new properties and constants related to Kubernetes, and tests are added to ensure the functionality of the new features. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant K8sManager as KubernetesManager
participant ConfigMap as Kubernetes ConfigMap
App->>K8sManager: createConfigMap()
K8sManager->>ConfigMap: API call to create ConfigMap
ConfigMap-->>K8sManager: ConfigMap created
K8sManager-->>App: ConfigMap created successfully
App->>K8sManager: loadFromConfigMap()
K8sManager->>ConfigMap: API call to load ConfigMap
ConfigMap-->>K8sManager: ConfigMap data
K8sManager-->>App: ConfigMap data retrieved
Tip OpenAI O1 model for chat
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (7)
apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (5)
38-48: Improve exception handling.Consider making the following improvements to the exception handling:
- Make the exception message more specific by including the actual exception message or cause.
- Log the error before throwing the exception to aid in debugging.
Apply this diff to implement the suggestions:
@PostConstruct public void initClient() { try { client = Config.defaultClient(); Configuration.setDefaultApiClient(client); coreV1Api = new CoreV1Api(client); } catch (Exception e) { - throw new RuntimeException("k8s client init failed"); + String errorMessage = "Failed to initialize Kubernetes client: " + e.getMessage(); + log.error(errorMessage, e); + throw new RuntimeException(errorMessage, e); } }
50-63: Improve error handling and add Javadoc comments.Consider making the following improvements to the method:
- Throw a custom exception (e.g.,
KubernetesClientException) in case of an error instead of returningnull. This will make it clear to the caller that an error occurred and needs to be handled.- Add Javadoc comments to document the method, including the parameters, return value, and exceptions thrown.
Apply this diff to implement the suggestions:
+/** + * Creates a Kubernetes ConfigMap. + * + * @param configMapNamespace the namespace of the ConfigMap + * @param name the name of the ConfigMap + * @param data the data to be stored in the ConfigMap + * @return the name of the created ConfigMap + * @throws KubernetesClientException if an error occurs while creating the ConfigMap + */ public String createConfigMap(String configMapNamespace, String name, Map<String, String> data) { if (configMapNamespace == null || configMapNamespace == "" || name == null || name == "") { log.error("create config map failed due to null parameter"); - return null; + throw new KubernetesClientException("ConfigMap namespace and name cannot be null or empty"); } V1ConfigMap configMap = new V1ConfigMap().metadata(new V1ObjectMeta().name(name).namespace(configMapNamespace)).data(data); try { coreV1Api.createNamespacedConfigMap(configMapNamespace, configMap, null, null, null,null); return name; } catch (Exception e) { log.error("create config map failed", e); - return null; + throw new KubernetesClientException("Failed to create ConfigMap: " + e.getMessage(), e); } }
65-87: Improve error handling, use constants for log messages, and add Javadoc comments.Consider making the following improvements to the method:
- Throw a custom exception (e.g.,
KubernetesClientException) in case of an error or if the data is not found instead of returningnull. This will make it clear to the caller that an error occurred and needs to be handled.- Use constants for the log messages and avoid string concatenation to improve performance and readability.
- Add Javadoc comments to document the method, including the parameters, return value, and exceptions thrown.
Apply this diff to implement the suggestions:
+private static final String ERROR_PARAMETER_CANNOT_BE_EMPTY = "参数不能为空"; +private static final String ERROR_CONFIG_MAP_NOT_FOUND = "ConfigMap不存在"; +private static final String ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP = "在ConfigMap中未找到指定的键: %s"; +/** + * Loads data from a Kubernetes ConfigMap. + * + * @param configMapNamespace the namespace of the ConfigMap + * @param name the name of the ConfigMap and the key to retrieve the data from + * @return the data associated with the name key in the ConfigMap + * @throws KubernetesClientException if an error occurs while loading the data or if the data is not found + */ public String loadFromConfigMap(String configMapNamespace, String name) { if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || name == null || name.isEmpty()) { - log.error("参数不能为空"); + log.error(ERROR_PARAMETER_CANNOT_BE_EMPTY); - return null; + throw new KubernetesClientException(ERROR_PARAMETER_CANNOT_BE_EMPTY); } try { V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null); if (configMap == null) { - log.error("ConfigMap不存在"); + log.error(ERROR_CONFIG_MAP_NOT_FOUND); - return null; + throw new KubernetesClientException(ERROR_CONFIG_MAP_NOT_FOUND); } Map<String, String> data = configMap.getData(); if (data != null && data.containsKey(name)) { return data.get(name); } else { - log.error("在ConfigMap中未找到指定的键: " + name); + log.error(String.format(ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP, name)); - return null; + throw new KubernetesClientException(String.format(ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP, name)); } } catch (Exception e) { log.error("get config map failed", e); - return null; + throw new KubernetesClientException("Failed to load data from ConfigMap: " + e.getMessage(), e); } }
89-109: Improve error handling, use constants for log messages, and add Javadoc comments.Consider making the following improvements to the method:
- Throw a custom exception (e.g.,
KubernetesClientException) in case of an error or if the value is not found instead of returningnull. This will make it clear to the caller that an error occurred and needs to be handled.- Use constants for the log messages and avoid string concatenation to improve performance and readability.
- Add Javadoc comments to document the method, including the parameters, return value, and exceptions thrown.
Apply this diff to implement the suggestions:
+private static final String ERROR_PARAMETER_CANNOT_BE_EMPTY = "参数不能为空"; +private static final String ERROR_CONFIG_MAP_NOT_FOUND_OR_NO_DATA = "ConfigMap不存在或没有数据"; +private static final String ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP = "在ConfigMap中未找到指定的键: %s"; +/** + * Retrieves a specific value from a Kubernetes ConfigMap. + * + * @param configMapNamespace the namespace of the ConfigMap + * @param name the name of the ConfigMap + * @param key the key to retrieve the value from + * @return the value associated with the key in the ConfigMap + * @throws KubernetesClientException if an error occurs while retrieving the value or if the value is not found + */ public String getValueFromConfigMap(String configMapNamespace, String name, String key) { if (configMapNamespace == null || configMapNamespace.isEmpty() || name == null || name.isEmpty() || key == null || key.isEmpty()) { - log.error("参数不能为空"); + log.error(ERROR_PARAMETER_CANNOT_BE_EMPTY); - return null; + throw new KubernetesClientException(ERROR_PARAMETER_CANNOT_BE_EMPTY); } try { V1ConfigMap configMap = coreV1Api.readNamespacedConfigMap(name, configMapNamespace, null); if (configMap == null || configMap.getData() == null) { - log.error("ConfigMap不存在或没有数据"); + log.error(ERROR_CONFIG_MAP_NOT_FOUND_OR_NO_DATA); - return null; + throw new KubernetesClientException(ERROR_CONFIG_MAP_NOT_FOUND_OR_NO_DATA); } if (!configMap.getData().containsKey(key)) { - log.error("在ConfigMap中未找到指定的键: " + key); + log.error(String.format(ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP, key)); - return null; + throw new KubernetesClientException(String.format(ERROR_KEY_NOT_FOUND_IN_CONFIG_MAP, key)); } return configMap.getData().get(key); } catch (Exception e) { log.error("get config map failed", e); - return null; + throw new KubernetesClientException("Failed to retrieve value from ConfigMap: " + e.getMessage(), e); } }
111-124: Improve error handling, use constants for log messages, and add Javadoc comments.Consider making the following improvements to the method:
- Throw a custom exception (e.g.,
KubernetesClientException) in case of an error instead of returningnull. This will make it clear to the caller that an error occurred and needs to be handled.- Use constants for the log messages and avoid string concatenation to improve performance and readability.
- Add Javadoc comments to document the method, including the parameters, return value, and exceptions thrown.
Apply this diff to implement the suggestions:
+private static final String ERROR_PARAMETER_CANNOT_BE_EMPTY = "参数不能为空"; +/** + * Updates a Kubernetes ConfigMap. + * + * @param configMapNamespace the namespace of the ConfigMap + * @param name the name of the ConfigMap + * @param data the updated data to be stored in the ConfigMap + * @return the name of the updated ConfigMap + * @throws KubernetesClientException if an error occurs while updating the ConfigMap + */ public String updateConfigMap(String configMapNamespace, String name, Map<String, String> data) { if (configMapNamespace == null || configMapName </blockquote></details> <details> <summary>apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (1)</summary><blockquote> `25-25`: **Avoid wildcard imports.** Importing all classes from a package using the wildcard `*` is generally discouraged as it can lead to naming conflicts and make the code less readable. Please explicitly import only the required classes from the `com.ctrip.framework.apollo.internals` package. </blockquote></details> <details> <summary>apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1)</summary><blockquote> `239-255`: **LGTM, but consider adding a TODO comment for the upstream data recovery logic.** The method correctly attempts to synchronize from the upstream repository and updates the `configMapProperties` if successful. It also catches any exceptions that occur during the synchronization process and logs an error message. Consider adding a TODO comment to indicate that the logic for recovering data from the upstream needs to be implemented: ```java // TODO: Implement logic to recover data from the upstream
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- apollo-client/pom.xml (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/enums/ConfigSourceType.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (4 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (6 hunks)
- apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (1 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java (2 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (1 hunks)
- apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/Env.java (1 hunks)
Additional comments not posted (37)
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ConfigConsts.java (1)
27-27: LGTM!The new constant
KUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULTis declared correctly with an appropriate name and value. The addition of this constant aligns with the PR objective of supporting Kubernetes ConfigMap for the Apollo client.apollo-client/src/main/java/com/ctrip/framework/apollo/enums/ConfigSourceType.java (1)
25-28: LGTM!The addition of the
CONFIGMAPentry to theConfigSourceTypeenum is a valid change that aligns with the PR objective of supporting Kubernetes ConfigMap as a new persistent storage mechanism for the Apollo client.This change enhances the flexibility of the configuration management system by providing an additional source type for loading configurations from Kubernetes ConfigMaps, enabling better integration with Kubernetes environments.
apollo-core/src/main/java/com/ctrip/framework/apollo/core/enums/Env.java (1)
38-38: LGTM!The addition of the
KUBERNETESenum value to theEnvenum is a valid change that aligns with the PR objective of supporting Kubernetes ConfigMap. This is a non-breaking change that extends the enum to accommodate a new environment type without altering any existing logic or control flow. The code change is small, self-contained, and does not require any additional unit tests.apollo-client/pom.xml (1)
101-105: Dependency addition aligns with the PR objective. Verify compatibility.The addition of the Kubernetes Java client library dependency is consistent with the goal of integrating Kubernetes support into the Apollo client. It enables the project to interact with Kubernetes resources programmatically.
Please ensure that version
18.0.0is the latest stable release and is compatible with the existing dependencies in the project. You can run the following script to check for any newer versions and potential compatibility issues:apollo-client/src/main/resources/META-INF/additional-spring-configuration-metadata.json (1)
79-92: LGTM!The new configuration properties for Kubernetes ConfigMap caching are correctly defined with appropriate types, default values, and descriptions. They follow the existing naming convention and are added in the correct alphabetical order within the
propertiesarray. ThesourceTypeis correctly set, and the default values are appropriate for the respective types and use cases. The descriptions provide clear information about the purpose and usage of the properties.Great job on enhancing the configuration options for managing caching behavior in Kubernetes environments!
apollo-core/src/main/java/com/ctrip/framework/apollo/core/ApolloClientSystemConsts.java (2)
76-84: LGTM!The new constants
APOLLO_CONFIGMAP_NAMESPACEandAPOLLO_CONFIGMAP_NAMESPACE_ENVIRONMENT_VARIABLESare well-named, follow the existing naming convention, and have appropriate access modifiers. The comments provide a clear explanation of their purpose related to Kubernetes ConfigMap namespace configuration.
170-178: LGTM!The new constants
APOLLO_KUBERNETES_CACHE_ENABLEandAPOLLO_KUBERNETES_CACHE_ENABLE_ENVIRONMENT_VARIABLESare well-named, follow the existing naming convention, and have appropriate access modifiers. The comments provide a clear explanation of their purpose related to enabling property names cache in Kubernetes environments.apollo-client/src/main/java/com/ctrip/framework/apollo/spi/DefaultConfigFactory.java (3)
105-108: LGTM!The changes introduce a new configuration option to enable Kubernetes ConfigMap caching and the logic to create the appropriate config repository based on the cache settings is implemented correctly. The TODO comment is a valid suggestion for future enhancement to allow both local and ConfigMap caching simultaneously.
130-143: LGTM!The new
createConfigMapConfigRepositorymethod is implemented correctly to enable creating a Kubernetes ConfigMap repository for a given namespace. The logic to create the repository based on the Kubernetes mode and using the local config repository as fallback when not in Kubernetes mode is a good approach for backward compatibility.
144-144: LGTM!The additional blank line after the
createConfigMapConfigRepositorymethod improves code readability.apollo-client/src/test/java/com/ctrip/framework/apollo/util/ConfigUtilTest.java (2)
246-255: LGTM!The test method
testConfigMapNamespaceWithSystemPropertyis well-structured and correctly verifies the behavior of retrieving the configuration map namespace from a system property. The test sets the system property, creates an instance ofConfigUtil, and asserts that the value returned bygetConfigMapNamespace()matches the expected value.
257-262: LGTM!The test method
testConfigMapNamespaceWithDefaultis well-structured and correctly verifies the behavior of retrieving the default configuration map namespace when no system property is set. The test creates an instance ofConfigUtiland asserts that the value returned bygetConfigMapNamespace()matches the default value defined inConfigConsts.KUBERNETES_CACHE_CONFIG_MAP_NAMESPACE_DEFAULT.apollo-client/src/test/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManagerTest.java (8)
51-69: LGTM!The test method
testCreateConfigMapSuccessis correctly implemented and covers the successful scenario for creating a ConfigMap. It mocks theCoreV1Apiand verifies that thecreateNamespacedConfigMapmethod is called with the correct arguments.
75-88: LGTM!The test method
testCreateConfigMapEmptyNamespaceis correctly implemented and covers the scenario of creating a ConfigMap with an empty namespace. It verifies that thecreateNamespacedConfigMapmethod is never called and the result is null.
93-107: LGTM!The test method
testCreateConfigMapEmptyNameis correctly implemented and covers the scenario of creating a ConfigMap with an empty name. It verifies that thecreateNamespacedConfigMapmethod is never called and the result is null.
112-125: LGTM!The test method
testCreateConfigMapNullDatais correctly implemented and covers the scenario of creating a ConfigMap with null data. It verifies that thecreateNamespacedConfigMapmethod is called once and the result is the same as the input name.
130-144: LGTM!The test method
testLoadFromConfigMapSuccessis correctly implemented and covers the successful scenario for loading data from a ConfigMap. It mocks theCoreV1Apiand verifies that thereadNamespacedConfigMapmethod is called with the correct arguments and the result is the expected value.
149-161: LGTM!The test method
testLoadFromConfigMapFailureis correctly implemented and covers the scenario of loading data from a ConfigMap when an exception is thrown. It mocks theCoreV1Apito throw anApiExceptionand verifies that the result is null.
166-178: LGTM!The test method
testLoadFromConfigMapConfigMapNotFoundis correctly implemented and covers the scenario of loading data from a ConfigMap when the ConfigMap is not found. It mocks theCoreV1Apito return null and verifies that the result is null.
183-199: LGTM!The test method
testGetValueFromConfigMapReturnsValueis correctly implemented and covers the scenario of getting a value from a ConfigMap when the key exists. It mocks theCoreV1Apito return a ConfigMap with the expected key-value pair and verifies that the result is the expected value.apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (11)
76-78: LGTM!The constructor correctly delegates to another constructor with an additional null parameter.
80-91: LGTM!The constructor correctly initializes the necessary fields and calls the appropriate setter methods.
93-101: LGTM!The method correctly sets the
configMapKeyfield based on the providedclusterandnamespaceparameters.
103-109: LGTM!The method correctly sets the
configMapNamefield, checks its validity, and performs synchronization if necessary.
111-133: LGTM!The method correctly checks the validity of the
configMapName, creates the ConfigMap if it doesn't exist, and uses a transaction to track the creation.
135-143: LGTM!The method correctly returns the
configMapProperties, performs synchronization if necessary, and creates a newPropertiesinstance to avoid modifying the originalconfigMapProperties.
150-161: LGTM!The method correctly sets the
upstreamfield, removes the current instance as a listener from the previousupstream, and adds it as a listener to the newupstream.
172-200: LGTM!The method correctly attempts to synchronize from the upstream repository and falls back to loading the configuration from the Kubernetes ConfigMap if the upstream synchronization fails. It uses a transaction to track the loading process and throws an exception if the loaded
configMapPropertiesis null.
204-237: LGTM!The method correctly retrieves the configuration from the Kubernetes ConfigMap, decodes the Base64-encoded JSON string, converts it to a
Mapusing Gson, and copies the key-value pairs to a newPropertiesinstance. It also handles exceptions and throws anApolloConfigExceptionif an error occurs during the loading process.
263-271: LGTM!The method correctly updates the
configMapPropertieswith the new properties and fires a repository change event. It also avoids unnecessary updates if thenewPropertiesare equal to the currentconfigMapProperties.
282-310: LGTM!The method correctly converts the
propertiesto a Base64-encoded JSON string, creates aMapwith theappIdas the key and the encoded JSON string as the value, and updates the Kubernetes ConfigMap using thekubernetesManager. It also uses a transaction to track the update process and logs an error if an exception occurs.apollo-client/src/main/java/com/ctrip/framework/apollo/util/ConfigUtil.java (6)
371-379: LGTM!The
getConfigMapNamespacemethod is well-structured and provides a clear way to retrieve the Kubernetes config map namespace. It allows for customization via thegetCustomizedConfigMapNamespacemethod while falling back to a default namespace if no customization is provided.
381-397: LGTM!The
getCustomizedConfigMapNamespacemethod provides a robust way to retrieve a customized Kubernetes config map namespace. It checks multiple sources in a logical order, allowing for flexibility in how the namespace can be customized. The code is clean and easy to understand.
408-415: LGTM!The
isInKubernetesModemethod is a straightforward way to check if the application is running in a Kubernetes environment. It handles exceptions gracefully by catching and ignoring them, returning false in case of any unexpected errors. This ensures the method doesn't break the application flow.
511-513: LGTM!The
isPropertyKubernetesCacheEnabledmethod is a simple and properly named getter for thePropertyKubernetesCacheEnabledfield. It follows the convention for boolean getter methods.
537-541: LGTM!The
initPropertyKubernetesCacheEnabledmethod follows the established pattern for initializing boolean properties in theConfigUtilclass. It utilizes thegetPropertyBooleanmethod to handle the retrieval and parsing of the Kubernetes cache enable property value from various sources. The code is consistent and maintainable.
74-74: LGTM!The
PropertyKubernetesCacheEnabledfield is declared with the appropriate access modifier and naming convention. The default value of false is a sensible choice. The field serves a clear purpose in theConfigUtilclass.
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Actionable comments posted: 1
Outside diff range and nitpick comments (4)
apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (4)
76-108: Translate error messages to English for consistency.The
loadFromConfigMapmethod correctly implements the retrieval of data from a Kubernetes ConfigMap. It performs necessary validations on the input parameters and handles exceptions appropriately.However, the error messages are in Chinese, while the rest of the codebase uses English. Consider translating the error messages to English for consistency.
Apply this diff to translate the error messages:
-log.error("参数不能为空"); +log.error("Parameters cannot be null or empty"); -log.error("ConfigMap不存在"); +log.error("ConfigMap does not exist"); -log.error("在ConfigMap中未找到指定的键: " + name); +log.error("Specified key not found in ConfigMap: " + name);
110-137: Translate error messages to English for consistency.The
getValueFromConfigMapmethod correctly implements the retrieval of a specific value from a Kubernetes ConfigMap. It performs necessary validations on the input parameters and handles cases where the ConfigMap doesn't exist or the specified key is not found by returning null.However, the error messages are in Chinese, while the rest of the codebase uses English. Consider translating the error messages to English for consistency.
Apply this diff to translate the error messages:
-log.error("参数不能为空"); +log.error("Parameters cannot be null or empty"); -log.error("ConfigMap不存在或没有数据"); +log.error("ConfigMap does not exist or has no data"); -log.error("在ConfigMap中未找到指定的键: " + key); +log.error("Specified key not found in ConfigMap: " + key);
139-159: Translate error message to English for consistency.The
updateConfigMapmethod correctly implements the update of a Kubernetes ConfigMap. It performs necessary validations on the input parameters and handles the case where the update fails by returning null.However, the error message is in Chinese, while the rest of the codebase uses English. Consider translating the error message to English for consistency.
Apply this diff to translate the error message:
-log.error("参数不能为空"); +log.error("Parameters cannot be null or empty");
161-179: Translate error message to English for consistency.The
checkConfigMapExistmethod correctly implements the check for the existence of a Kubernetes ConfigMap. It performs necessary validations on the input parameters and handles the case where the ConfigMap doesn't exist or an exception occurs by returning false.However, the error message is in Chinese, while the rest of the codebase uses English. Consider translating the error message to English for consistency.
Apply this diff to translate the error message:
-log.error("参数不能为空"); +log.error("Parameters cannot be null or empty");
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (1 hunks)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java (1 hunks)
- apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- apollo-client/src/main/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepository.java
Additional comments not posted (6)
apollo-client/src/test/java/com/ctrip/framework/apollo/internals/K8sConfigMapConfigRepositoryTest.java (4)
73-87: LGTM!The test method correctly verifies the behavior of the
syncmethod when it successfully syncs from the upstream data source.
93-105: LGTM!The test method correctly verifies the behavior of the
syncmethod when it fails to sync from the upstream data source but successfully loads from the Kubernetes ConfigMap.
126-136: LGTM!The test method correctly verifies that an
ApolloConfigExceptionis thrown when theloadFromK8sConfigMapmethod encounters an exception while loading the configuration from the Kubernetes ConfigMap.
142-152: LGTM!The test method correctly verifies that the
persistConfigMapmethod successfully persists the configuration to the Kubernetes ConfigMap by checking that theupdateConfigMapmethod is called once on theKubernetesManagerwith the expected arguments.apollo-client/src/main/java/com/ctrip/framework/apollo/Kubernetes/KubernetesManager.java (2)
38-50: LGTM!The
initClientmethod correctly initializes the Kubernetes client using the default configuration. It also handles exceptions appropriately by logging the error and throwing aRuntimeException.
52-74: LGTM!The
createConfigMapmethod correctly implements the creation of a Kubernetes ConfigMap. It performs necessary validations on the input parameters, handles exceptions appropriately, and provides a clear Javadoc comment.
| public void testLoadFromK8sConfigMapSuccess() throws Throwable { | ||
| // arrange | ||
| when(kubernetesManager.getValueFromConfigMap(anyString(), anyString(), anyString())).thenReturn("encodedConfig"); | ||
|
|
||
| // act | ||
| Properties properties = k8sConfigMapConfigRepository.loadFromK8sConfigMap(); | ||
|
|
||
| // assert | ||
| verify(kubernetesManager, times(1)).getValueFromConfigMap(anyString(), anyString(), anyString()); | ||
| // 这里应该有更具体的断言来验证properties的内容，但由于编码和解码逻辑未给出，此处省略 | ||
| } |
There was a problem hiding this comment.
Add assertions for the loaded properties.
The test method verifies that the getValueFromConfigMap method is called once on the KubernetesManager. However, it's missing assertions to verify the content of the loaded properties.
Please add assertions to verify that the loaded properties match the expected values based on the mocked encodedConfig.
What's the purpose of this PR
解决Apollo客户端在Kubernetes环境下因服务端宕机或Pod重启导致配置信息文件丢失的问题。通过使用Kubernetes ConfigMap作为新的持久化存储方案，提高配置信息的可靠性和容错性。
Solve the problem of Apollo client configuration information files being lost due to server downtime or Pod restart in the Kubernetes environment. By using Kubernetes ConfigMap as a new persistent storage solution, the reliability and fault tolerance of configuration information are improved.
discussion apolloconfig/apollo#5210
Brief changelog
XXXXX
Follow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
New Features
KubernetesManagerclass for creating, reading, updating, and checking ConfigMaps.Bug Fixes
Tests
KubernetesManagerandK8sConfigMapConfigRepository.Documentation